Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds warning logs to detect mixed-case _cf_ prefix usage in SQL identifiers and FTS5 virtual tables.
Issues
-
[High]
<strings.h>included unconditionally — breaks Windows/MSVC build.
strncasecmpis a POSIX function declared in<strings.h>, which doesn't exist on MSVC. Other files in the codebase handle this:form-data.c++guards the include with#if !_MSC_VER, andsqlite.c++uses#define strncasecmp _strnicmp. The new code insql.c++does neither. -
[Low] Log messages don't include the offending identifier.
SinceLOG_WARNING_PERIODICALLYis rate-limited to once per hour per call site, including the actual name would be much more useful for debugging when these fire in production. Both call sites have the name readily available.
|
Review posted successfully on PR #6448. Here's a summary of the findings:
All three issues have concrete suggestion comments on the PR. |
maxmcd
force-pushed
the
mmcdonnell/cf-table-prefix-logs
branch
2 times, most recently
from
5fb5497 to
f0f2b43
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6448 +/- ##
=======================================
Coverage 70.91% 70.91%
=======================================
Files 428 428
Lines 119919 119927 +8
Branches 18996 19002 +6
=======================================
+ Hits 85035 85041 +6
- Misses 23597 23600 +3
+ Partials 11287 11286 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maxmcd
force-pushed
the
mmcdonnell/cf-table-prefix-logs
branch
from
f0f2b43 to
3bd6994
Compare
maxmcd
force-pushed
the
mmcdonnell/cf-table-prefix-logs
branch
from
3bd6994 to
c084caf
Compare
3 participants
Add warning logs to detect SQL identifiers using mixed-case cf prefix variants (e.g. CF, Cf) and FTS5 virtual tables using any-case cf prefix.